Conversation
Why? just makes review more difficult. way too many changes. |
....wurstscript/src/main/java/de/peeeq/wurstio/intermediateLang/interpreter/ProgramStateIO.java
Outdated
Show resolved
Hide resolved
| if (e instanceof ExprNewObject) { | ||
| ExprNewObject enew = (ExprNewObject) e; | ||
| if (e.isEmpty()) { | ||
| // TODO non simple TypeRef |
There was a problem hiding this comment.
Where did this line come from?
There was a problem hiding this comment.
Ah it's copy-pasted from the final return in this function. Unclear to me if the TODO is relevant here as well.
|
|
||
| if (elem instanceof ExprIntVal | ||
| && elem.getParent() != null) { | ||
| && Optional.ofNullable(elem.getParent()).isPresent()) { |
There was a problem hiding this comment.
@peq I see that Wscope.java is AST-generated. Is there any way to make it return Optional for getParent?
Hey @Frotty thanks for the comment. I agree, it's not good that it ended up this way. I was going down a rabbit hole and elected not to think about it much and just kept digging. I am hoping that it won't be too contentious since commit volume on the compiler is relatively low. |
|
yes, target is 8. |
|
Thanks @Frotty I didn't realise that Java works in a way that sourceCompatibility is ignored during compile time. I will push a commit to fix this shortly. |
|
|
||
| private class TestTimeOutException extends Throwable { | ||
|
|
||
| private static final long serialVersionUID = -7080085729129882874L; |
There was a problem hiding this comment.
It's a generated value for a serialisable class. IDE flagged it, I'm not sure if it's actually needed or not
There was a problem hiding this comment.
It's a generated value for a serialisable class
Is this class serialized?
There was a problem hiding this comment.
I don't know sorry, but my language server flagged it, so maybe yes?
| import java.io.IOException; | ||
| import java.util.Optional; | ||
|
|
||
| public class W3Utils { |
There was a problem hiding this comment.
Is there any circumstance where this class not being a singleton is required?
There was a problem hiding this comment.
It wasn't a singleton before, it was static
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/languageserver/requests/MapRequest.java
Outdated
Show resolved
Hide resolved
|
I guess this could be nice, but it really contains too many irrelevant changes for the actual content to be properly reviewed. |
|
I disagree, I reviewed it myself in one evening. There is a lot of repetition and the interesting content is sparse I suggest to use this tooling to make it more digestible:
|
|
Hey, I didn't see any comments here in a while. It seems that review is stalling here because it's not clear who is expected to review and also because of the burden of high number of changes. I can do some surgery and PR one change at a time, but before spending time on that it would be good to know what changes people are on-board with and if it will even get a review on that form. @Frotty @peq are you prepared to spend time on reviewing? Am I right that splitting this up will get 👀 sooner? Are there any changes here that you're not on-board with? I do not think this should be needed tbh. You should just review this PR - it's not even 1000 lines. |
True, then it should be no problem for you to separate features from refactorings 👍 |
|
No, it would be quite painful. These changes are not purely orthogonal. I think the onus is on you the maintainers to at least review this at a high level. I have provided bullet points of the 5 main changes. |
|
I made the missing changes on another branch because github desktop didn't let me push to this one. |
😱
In updating w3libs for reforged I took a look at how WurstScript uses it, and wasn't quite satisfied that updating w3libs would be sufficient to improve stability.
This PR bundles a few things all in one 💩:
git(wasn't working for me in vscode so I rewrote to use jgit)Optionalinstead of @nullable and @notnulll